Skip to content

Macpython 32 bit build fixup #34416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
May 29, 2020
Merged

Macpython 32 bit build fixup #34416

merged 6 commits into from
May 29, 2020

Conversation

TomAugspurger
Copy link
Contributor

Closes #34114

cc @jbrockmendel and @WillAyd. I have no idea if this is correct, but it builds.

@TomAugspurger
Copy link
Contributor Author

I looked briefly at adding these builds to our CI, but didn't make any progress. I think it comes down to including a

container: quay.io/pypa/manylinux1_i686:latest

At the same level as job and pool in the yaml file.

@TomAugspurger TomAugspurger added 32bit 32-bit systems Build Library building on various platforms labels May 27, 2020
@TomAugspurger TomAugspurger added this to the 1.1 milestone May 27, 2020
@jbrockmendel
Copy link
Member

the tokenizer.c edit looks good and the period.pyx edit looks harmless.

But Py_ssize_t -> khiter_t looks sketchy in a subset of the places the change is made; ill comment in-line.

@jbrockmendel
Copy link
Member

I'm OK with merging this as-is if this is a blocker for wheel building or 1.0.4 or something

WillAyd
WillAyd reviewed May 28, 2020
View reviewed changes
pandas/_libs/tslibs/period.pyx
@@ -73,7 +73,7 @@ from pandas._libs.tslibs.tzconversion cimport tz_convert_utc_to_tzlocal

cdef:
enum:
INT32_MIN = -2_147_483_648
INT32_MIN = -2_147_483_648LL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
INT32_MIN = -2_147_483_648LL
INT32_MIN = -2_147_483_648L

Rather pedantic but the size of a long is at least 32 bits, so no need for long long here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI here's some interesting info on why we probably need this suffix depending on the architecture / compilation standard:

https://stackoverflow.com/questions/9941261/warning-this-decimal-constant-is-unsigned-only-in-iso-c90#20910712

Copy link
Member

@WillAyd WillAyd May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ignore original comment; I think LL might be correct here

@WillAyd WillAyd mentioned this pull request May 28, 2020
Merged
4 tasks
@TomAugspurger TomAugspurger merged commit 1cad9e5 into pandas-dev:master May 29, 2020
@TomAugspurger TomAugspurger deleted the macpython-32-bit-build-fixup branch May 29, 2020 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@WillAyd WillAyd WillAyd approved these changes

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
32bit 32-bit systems Build Library building on various platforms
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.

MacPython linux build failing
4 participants
@TomAugspurger @jbrockmendel @WillAyd @jreback